Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(prometheus) prevent error to use $__interval_ms in query #12533

Merged
merged 3 commits into from Jul 11, 2018
Merged

(prometheus) prevent error to use $__interval_ms in query #12533

merged 3 commits into from Jul 11, 2018

Conversation

mtanda
Copy link
Collaborator

@mtanda mtanda commented Jul 9, 2018

$__interval_ms type is integer. It cause error.

@torkelo
Copy link
Member

torkelo commented Jul 9, 2018

Can you rebase this on latest master?

@davkal
Copy link
Contributor

davkal commented Jul 9, 2018

This function could use a comment and a test. Currently I have no idea why it's there and what input/output is typical.

torkelo
torkelo reviewed Jul 9, 2018
View reviewed changes
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mtanda . A test for this and maybe a comment what this function does. Should this function not always return a string?

@mtanda
Copy link
Collaborator Author

mtanda commented Jul 9, 2018

Sorry, I'm not sure what I should do.

There are tests for prometheusRegularEscape().
https://github.com/grafana/grafana/blob/v5.2.1/public/app/plugins/datasource/prometheus/specs/datasource.jest.ts#L168-L178

I think prometheusRegularEscape() should return string type.
So I change the code, convert number to string in caller side.

@davkal
Copy link
Contributor

davkal commented Jul 10, 2018

@mtanda we would like to use opportunities where an error was encountered to extend our unit tests.
In this instance it happened in interpolateQueryExpr. Could we ask you to please add a test suite for interpolateQueryExpr() inside datasource.jest.ts as part of this patch? Ideally it would contain a test that would fail before your fix (and pass with the fix).

@mtanda
Copy link
Collaborator Author

mtanda commented Jul 10, 2018

I understand what you need. I add a test case for failure pattern.

torkelo
torkelo reviewed Jul 10, 2018
View reviewed changes
public/app/plugins/datasource/prometheus/datasource.ts Outdated
@@ -103,6 +103,10 @@ export class PrometheusDatasource {
interpolateQueryExpr(value, variable, defaultFormatFn) {
// if no multi or include all do not regexEscape
if (!variable.multi && !variable.includeAll) {
if (typeof value === 'number') {
// $__interval_ms type is number, convert to string here
value = value.toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure this is the correct place to solve this, maybe the root issue should be fixed instead? wonder if allowing $__interval_ms to have number value is not correct but it should be turned into string when it is save in scopedVars (in metrics_ctrl & prometheus datasource.ts)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I little hesitate to break the current behavior, but I agree the data type should be string.

@torkelo torkelo merged commit 18a8290 into grafana:master Jul 11, 2018
ryantxu added a commit to ryantxu/grafana that referenced this pull request Jul 12, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master'
14d816a 
* grafana/master: (29 commits)
  skip backend request if extended statistics is invalid. (grafana#12495)
  Refactor team pages to react & design change (grafana#12574)
  (prometheus) prevent error to use $__interval_ms in query (grafana#12533)
  fix: folder picker did not notify parent that the initial folder had been changed, fixes grafana#12543 (grafana#12554)
  Add support for skipping variable value in URL, fixes grafana#12174 (grafana#12541)
  Don't build-all for PRs
  fix: requests/sec instead of requets (grafana#12557)
  Add folder name to dashboard title (grafana#12545)
  Fix css loading in plugins (grafana#12573)
  Add new sequential color scales
  move go vet out of scripts and fixing warning (grafana#12552)
  Cleanup and remove some jest.fn()
  Remove irrelevant tests and templateSrv stub
  Update CHANGELOG.md
  fix diff and percent_diff (grafana#12515)
  Update lodash/moment version (grafana#12532)
  Add mock to test files
  Create new instance in beforeEach
  Remove comments
  Karma to Jest: Cloudwatch datasource
  ...
ryantxu added a commit to ryantxu/grafana that referenced this pull request Jul 12, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master' into panel-visibility-flag
966bbca 
* grafana/master: (30 commits)
  skip backend request if extended statistics is invalid. (grafana#12495)
  Refactor team pages to react & design change (grafana#12574)
  (prometheus) prevent error to use $__interval_ms in query (grafana#12533)
  fix: folder picker did not notify parent that the initial folder had been changed, fixes grafana#12543 (grafana#12554)
  Add support for skipping variable value in URL, fixes grafana#12174 (grafana#12541)
  Don't build-all for PRs
  fix: requests/sec instead of requets (grafana#12557)
  Add folder name to dashboard title (grafana#12545)
  Fix css loading in plugins (grafana#12573)
  Add new sequential color scales
  move go vet out of scripts and fixing warning (grafana#12552)
  Cleanup and remove some jest.fn()
  Remove irrelevant tests and templateSrv stub
  Update CHANGELOG.md
  fix diff and percent_diff (grafana#12515)
  Update lodash/moment version (grafana#12532)
  Tabs to spaces in tslint (grafana#12529)
  Add mock to test files
  Create new instance in beforeEach
  Remove comments
  ...
ryantxu added a commit to NatelEnergy/grafana that referenced this pull request Jul 12, 2018
@ryantxu
Merge remote-tracking branch 'grafana/master'
4ffc589 
* grafana/master: (30 commits)
  skip backend request if extended statistics is invalid. (grafana#12495)
  Refactor team pages to react & design change (grafana#12574)
  (prometheus) prevent error to use $__interval_ms in query (grafana#12533)
  fix: folder picker did not notify parent that the initial folder had been changed, fixes grafana#12543 (grafana#12554)
  Add support for skipping variable value in URL, fixes grafana#12174 (grafana#12541)
  Don't build-all for PRs
  fix: requests/sec instead of requets (grafana#12557)
  Add folder name to dashboard title (grafana#12545)
  Fix css loading in plugins (grafana#12573)
  Add new sequential color scales
  move go vet out of scripts and fixing warning (grafana#12552)
  Cleanup and remove some jest.fn()
  Remove irrelevant tests and templateSrv stub
  Update CHANGELOG.md
  fix diff and percent_diff (grafana#12515)
  Update lodash/moment version (grafana#12532)
  Tabs to spaces in tslint (grafana#12529)
  Add mock to test files
  Create new instance in beforeEach
  Remove comments
  ...
@torkelo torkelo added this to the 5.2.2 milestone Jul 13, 2018
@torkelo torkelo mentioned this pull request Jul 13, 2018
Closed
@davkal davkal mentioned this pull request Jul 16, 2018
Merged
marefr added a commit that referenced this pull request Jul 24, 2018
@marefr
changelog: add notes about closing #12533
1c691ac 
[skip ci]
marefr pushed a commit that referenced this pull request Jul 25, 2018
@mtanda @marefr
(prometheus) prevent error to use $__interval_ms in query (#12533)
534ba6d 
* prevent error to use $__interval_ms in query

* add test

* prevent error to use $__interval_ms in query

(cherry picked from commit 18a8290)
marefr added a commit that referenced this pull request Jul 25, 2018
@marefr
changelog: add notes about closing #12533
7e773e2 
[skip ci]
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torkelo torkelo torkelo left review comments

Assignees
No one assigned
Labels
pr/external This PR is from external contributor
Projects
None yet
Milestone
5.2.2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mtanda @torkelo @davkal @marefr @ying-jeanne